-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Shellcheck some test scripts #10797
Shellcheck some test scripts #10797
Conversation
af6c6d0
to
7043fad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These files aren't executable, and the presence of a shebang would suggest that that's by mistake.
I'd prefer the shellcheck hint, # shellcheck shell=bash
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually we could come up with a proper shebang, but if the interpreter is a script, it doesn't work on mac, so we might need a helper in the dev shell.
Also shellcheck wouldn't recognize the helper as bash, so we're going to need the hint anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roberth I think we could just make them executable. They are self-contained unlike the fragments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's quite right.
We should then also apply the bash options from common-test.sh
, and always go through the shebang instead of a non-standard invocation with $BASH
.
Even then, I'm not sure running tests without TESTS_ENVIRONMENT
is a sufficiently non-bad idea that we should seduce contributors into running them this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I would rather just get rid of TESTS_ENVIRONMENT
! :)
82c4e99
to
0ab64e1
Compare
Progress on NixOS#10795
0ab64e1
to
2e12b58
Compare
This is good for shebang, and also good for future build system simplifications
Motivation
See the issue.
Context
Progress on #10795
Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.